Fix security_groups for c8/suse by davidjumani · Pull Request #5354 · apache/cloudstack

davidjumani · 2021-08-23T06:30:27Z

Description

Fixes fetching the bridge name for c8 / suse where the result of virsh domlist contains leading spaces
Used only in component tests, so not a functionality break

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Bug Severity

How Has This Been Tested?

Before :

# virsh domiflist i-2-3-VM |grep -w '02:00:12:b0:00:01'
 vnet6       bridge   cloudbr1   virtio   02:00:12:b0:00:01    <- notice the leading space
# virsh domiflist i-2-3-VM |grep -w '02:00:12:b0:00:01' |tr -s ' '|cut -d ' ' -f3
bridge   <- Displays the second field

After :

# virsh domiflist i-2-3-VM |grep -w '02:00:12:b0:00:01'
 vnet6       bridge   cloudbr1   virtio   02:00:12:b0:00:01    <- notice the leading space
# virsh domiflist i-2-3-VM |grep -w '02:00:12:b0:00:01' | awk '{print $3}'
cloudbr1    <- Displays the bridge name

davidjumani · 2021-08-23T06:30:44Z

@blueorangutan package

blueorangutan · 2021-08-23T06:31:03Z

@davidjumani a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-08-23T06:57:02Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 972

weizhouapache

tested on a centos7 env, it works.

Create Vm with multiple NIC's ... === TestName: test_01_create_vm_with_multiple_nics | Status : SUCCESS ===
ok
Create VM with single NIC and then add additional NIC ... === TestName: test_02_add_nic_to_vm | Status : SUCCESS ===
ok
Add secondary IP's to the VM ... === TestName: test_03_add_ip_to_default_nic | Status : SUCCESS ===
ok
Add secondary IP's to remaining NIC's ... === TestName: test_04_add_ip_to_remaining_nics | Status : SUCCESS ===
ok
Stop and Start a VM with Multple NIC ... === TestName: test_05_stop_start_vm_with_multiple_nic | Status : SUCCESS ===
ok
Migrate a VM with Multple NIC ... === TestName: test_06_migrate_vm_with_multiple_nic | Status : SUCCESS ===
ok
Remove secondary IP from any NIC ... === TestName: test_07_remove_secondary_ip_from_nic | Status : SUCCESS ===
ok
Remove NIC from VM ... === TestName: test_08_remove_nic_from_vm | Status : SUCCESS ===
ok
Reboot a VM with Multple NIC ... === TestName: test_09_reboot_vm_with_multiple_nic | Status : SUCCESS ===
ok

----------------------------------------------------------------------
Ran 9 tests in 294.765s

OK

weizhouapache · 2021-08-23T07:44:54Z

@davidjumani @DaanHoogland
I reverted a minor change in #5348 in this pr.

@davidjumani could you target this pr to 4.15 ?

the issue also exist in ubuntu 20.04, I will create a testing env and run the test (it should work)

davidjumani · 2021-08-23T07:55:54Z

@blueorangutan package

blueorangutan · 2021-08-23T07:57:03Z

@davidjumani a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

davidjumani · 2021-08-23T07:57:55Z

Thanks @weizhouapache I also had to force push since I needed to rebase, can you add your changes again

yadvr

LGTM -haven't tested it, we'll need to test on all support distros to confirm this.

blueorangutan · 2021-08-23T08:23:14Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 973

davidjumani · 2021-08-23T08:32:15Z

@rhtyd This is used only for verifying the rules for tests, does not affect the functioning of the agent

weizhouapache · 2021-08-23T08:41:57Z

Thanks @weizhouapache I also had to force push since I needed to rebase, can you add your changes again

@davidjumani thanks. done.

weizhouapache · 2021-08-23T08:42:56Z

@rhtyd This is used only for verifying the rules for tests, does not affect the functioning of the agent

yes, agree with @davidjumani . it does not impact the functionalities.
it is only used by component test test/integration/component/test_multiple_nic_support.py

yadvr · 2021-08-24T07:12:43Z

LGTM then and based on Wei's tests too.
@blueorangutan package

blueorangutan · 2021-08-24T07:13:03Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

yadvr · 2021-08-24T07:30:01Z

Nevermind - reviewed this, LGTM. No tests needed as manual tests are provided and BO doesn't run component test with security groups (yet).

blueorangutan · 2021-08-24T08:01:11Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 992

davidjumani requested a review from weizhouapache August 23, 2021 06:30

weizhouapache approved these changes Aug 23, 2021

View reviewed changes

Fix security_groups for c8/suse

39c6848

davidjumani force-pushed the fix-sg branch from 6dfc702 to 39c6848 Compare August 23, 2021 07:55

davidjumani changed the base branch from main to 4.15 August 23, 2021 07:55

yadvr approved these changes Aug 23, 2021

View reviewed changes

yadvr added this to the 4.15.2.0 milestone Aug 23, 2021

security_group.py: revert a minor change in apache#5348

a0d8a4f

weizhouapache closed this Aug 23, 2021

weizhouapache reopened this Aug 23, 2021

yadvr merged commit f822547 into apache:4.15 Aug 24, 2021

Conversation

davidjumani commented Aug 23, 2021

Description

Types of changes

Bug Severity

How Has This Been Tested?

Before :

After :

Uh oh!

davidjumani commented Aug 23, 2021

Uh oh!

blueorangutan commented Aug 23, 2021

Uh oh!

blueorangutan commented Aug 23, 2021

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Aug 23, 2021

Uh oh!

davidjumani commented Aug 23, 2021

Uh oh!

blueorangutan commented Aug 23, 2021

Uh oh!

davidjumani commented Aug 23, 2021

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Aug 23, 2021

Uh oh!

davidjumani commented Aug 23, 2021

Uh oh!

weizhouapache commented Aug 23, 2021

Uh oh!

weizhouapache commented Aug 23, 2021

Uh oh!

yadvr commented Aug 24, 2021

Uh oh!

blueorangutan commented Aug 24, 2021

Uh oh!

yadvr commented Aug 24, 2021

Uh oh!

blueorangutan commented Aug 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants